Shorten the default Android keyframe interval to one second - #103
krystofwoldrich wants to merge 1 commit into
Conversation
Co-authored-by: Codex (GPT-6) <noreply@openai.com>
krystofwoldrich-agent
left a comment
krystofwoldrich-agent
left a comment
There was a problem hiding this comment.
🤖 This is an automated review. Addressing it doesn't guarantee a merge.
Review: Shorten the default Android keyframe interval to one second
Verdict: APPROVE
Findings: 0 critical · 0 design · 1 suggestion · 0 nit
The change lowers SCRCPY_DEFAULTS.keyFrameInterval from 10 to 1 and updates the CLI help, both README copies, the changeset, and the default-asserting tests. The default is consumed consistently: scrcpy passes it as i-frame-interval (seconds, MediaCodec semantics), and every FFmpeg backend in h264-encoder.ts derives keyint/-g from fps × keyFrameInterval, so all paths move together. The Hub's serve-emu-options.ts does not set keyFrameInterval, so the Hub inherits the new default, which makes the expo-device-hub changeset correct given that serve-emu is in the changeset ignore list. The generated README under packages/serve-emu/packages/serve-emu/ matches the source README row. Tests cover the scrcpy codec option and both gRPC encoder backends via the existing test.each.
Findings
[suggestion] Consider RTCP PLI handling as the targeted fix for WebRTC reference-frame loss
packages/serve-emu/packages/serve-emu/src/scrcpy.ts:137
webrtc-publisher.ts requests a keyframe on peer open, track open, and backpressure, and it wires an RtcpNackResponder, but I found no handling of Picture Loss Indication from the browser. That means a decoder that loses reference frames beyond what NACK retransmission covers has no way to ask for a recovery point, which matches the motivation in the PR description. A 1s GOP is a reasonable blanket mitigation, but at CBR (-maxrate/-bufsize equal to the bit rate) a keyframe every 60 frames takes a larger share of the 8 Mbps budget and lowers P-frame quality for static UI. Forwarding PLI to onKeyframeRequest would let the interval go back up later without losing recovery. Not blocking for this PR; the description already notes the experiment did not confirm keyframe recovery as the cause of the original freezes, so this is a follow-up idea.
| keyFrameInterval: 10, | ||
| // Frequent keyframes give decoders another recovery point after lost frames. | ||
| // Late joiners can still request an immediate keyframe via reset-video. | ||
| keyFrameInterval: 1, |
There was a problem hiding this comment.
[suggestion] webrtc-publisher.ts requests a keyframe on peer open, track open, and backpressure, and wires an RtcpNackResponder, but I found no handling of RTCP Picture Loss Indication from the browser. A decoder that loses reference frames beyond NACK recovery therefore has no way to ask for a recovery point, which matches this PR's motivation. The 1s GOP is a reasonable blanket mitigation, but at CBR (-maxrate/-bufsize equal to the bit rate) a keyframe every 60 frames takes a larger share of the 8 Mbps budget and lowers P-frame quality on static UI. Forwarding PLI to onKeyframeRequest would address the root cause and would let this interval go back up later. Not blocking.
2 participants
Android streams currently default to ten seconds between keyframes, which can leave a decoder waiting for the next recovery point after losing reference frames. Shorten the shared scrcpy and gRPC default to one second, update CLI help and the generated documentation, and add a patch changeset. Explicit interval overrides and on-demand keyframe requests remain unchanged.
For host FFmpeg encoding this is a frame-count interval: at a configured 60 FPS, the GOP changes from 600 to 60 frames. Actual wall-clock spacing grows when the source produces fewer frames. More frequent keyframes trade some compression efficiency for more frequent recovery opportunities; this does not change WebRTC feedback handling.
Validation:
bun run --filter serve-emu checkpassed: 973 tests, coverage checks, server/UI/test type checks, build, and package smoke test. Existing integration tests verify the one-second default for scrcpy and both gRPC encoder backends, including encoder replacement after a size change.-g 60and four keyframes in every 240-frame health window after warm-up.— Codex (GPT-6)